Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Ensure that Fingerprint caching respects hashing configuration #92278

Merged
merged 3 commits into from
Jan 10, 2022
Merged

Ensure that Fingerprint caching respects hashing configuration #92278

merged 3 commits into from
Jan 10, 2022
+118 −46

Conversation

Aaron1011
Copy link
Member

Fixes #92266

In some HashStable impls, we use a cache to avoid re-computing
the same Fingerprint from the same structure (e.g. an AdtDef).
However, the StableHashingContext used can be configured to
perform hashing in different ways (e.g. skipping Spans). This
configuration information is not included in the cache key,
which will cause an incorrect Fingerprint to be used if
we hash the same structure with different StableHashingContext
settings.

To fix this, the configuration settings of StableHashingContext
are split out into a separate HashingControls struct. This
struct is used as part of the cache key, ensuring that our caches
always produce the correct result for the given settings.

With this in place, we now turn off Span hashing during the
entire process of computing the hash included in legacy symbols.
This current has no effect, but will matter when a future PR
starts hashing more Spans that we currently skip.

@rustbot rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Dec 25, 2021
@rust-highfive
Copy link
Collaborator

r? @jackh726

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Dec 25, 2021
@Aaron1011 Aaron1011 mentioned this pull request Dec 25, 2021
Closed
@Aaron1011
Copy link
Member Author

Aaron1011 commented Dec 25, 2021 •
edited
Loading

cc @michaelwoerister @cjgillot

@Aaron1011
Copy link
Member Author

@bors try @rust-timer queue

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion.

@rustbot label: +S-waiting-on-perf

@rustbot rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Dec 25, 2021
@bors
Copy link
Contributor

bors commented Dec 25, 2021

⌛ Trying commit 0133c532074b7f9b3185f73069fd0efab8ad21ad with merge 4a8e40c5ad7cde9c3dba8f8633d09ed1df158899...

@rust-log-analyzer

This comment has been minimized.

Sign in to view
@bors
Copy link
Contributor

bors commented Dec 25, 2021

☀️ Try build successful - checks-actions
Build commit: 4a8e40c5ad7cde9c3dba8f8633d09ed1df158899 (4a8e40c5ad7cde9c3dba8f8633d09ed1df158899)

@rust-timer
Copy link
Collaborator

Queued 4a8e40c5ad7cde9c3dba8f8633d09ed1df158899 with parent c096176, future comparison URL.

@rust-timer
Copy link
Collaborator

Finished benchmarking commit (4a8e40c5ad7cde9c3dba8f8633d09ed1df158899): comparison url.

Summary: This change led to moderate relevant mixed results 🤷 in compiler performance.

  • Small improvement in instruction counts (up to -0.8% on incr-unchanged builds of externs)
  • Moderate regression in instruction counts (up to 1.7% on incr-unchanged builds of clap-rs)

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR led to changes in compiler perf.

Next Steps: If you can justify the regressions found in this try perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please fix the regressions and do another perf run. If the next run shows neutral or positive results, the label will be automatically removed.

@bors rollup=never
@rustbot label: +S-waiting-on-review -S-waiting-on-perf +perf-regression

@rustbot rustbot added perf-regression Performance regression. and removed S-waiting-on-perf Status: Waiting on a perf run to be completed. labels Dec 25, 2021
@Aaron1011
Copy link
Member Author

@bors try @rust-timer queue

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion.

@rustbot label: +S-waiting-on-perf

@rustbot rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Dec 27, 2021
@bors
Copy link
Contributor

bors commented Dec 27, 2021

⌛ Trying commit 900fec7d020395fabd3a2b8089a81a9b1fd5c8a9 with merge dd25572bf85b48f81c20ee8fffaac8f406d00af2...

@bors
Copy link
Contributor

bors commented Dec 27, 2021

💥 Test timed out

@rust-log-analyzer

This comment has been minimized.

Sign in to view
@Aaron1011
Copy link
Member Author

@bors try

@bors
Copy link
Contributor

bors commented Dec 27, 2021

⌛ Trying commit 900fec7d020395fabd3a2b8089a81a9b1fd5c8a9 with merge 48a976a697a2b214a52a74e580f2cd347cd15f9d...

@bors
Copy link
Contributor

bors commented Dec 27, 2021

☀️ Try build successful - checks-actions
Build commit: 48a976a697a2b214a52a74e580f2cd347cd15f9d (48a976a697a2b214a52a74e580f2cd347cd15f9d)

@rust-timer
Copy link
Collaborator

Queued 48a976a697a2b214a52a74e580f2cd347cd15f9d with parent f8abed9, future comparison URL.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 6, 2022
@bors
Copy link
Contributor

bors commented Jan 9, 2022

⌛ Testing commit 4ca275a with merge b431595d1630725bd11aabe6d349a937a2e9d37d...

@rust-log-analyzer
Copy link
Collaborator

The job dist-i686-linux failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)

@bors
Copy link
Contributor

bors commented Jan 9, 2022

💔 Test failed - checks-actions

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Jan 9, 2022
@Aaron1011
Copy link
Member Author

@bors retry

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 9, 2022
@bors
Copy link
Contributor

bors commented Jan 9, 2022

⌛ Testing commit 4ca275a with merge 98357a9e461aa033dfcb273430a6aa91a602ae04...

@bors
Copy link
Contributor

bors commented Jan 9, 2022

💔 Test failed - checks-actions

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Jan 9, 2022
@rust-log-analyzer
Copy link
Collaborator

A job failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)

@cjgillot
Copy link
Contributor

cjgillot commented Jan 9, 2022

@bors retry

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 9, 2022
@bors
Copy link
Contributor

bors commented Jan 10, 2022

⌛ Testing commit 4ca275a with merge d63a8d9...

@bors
Copy link
Contributor

bors commented Jan 10, 2022

☀️ Test successful - checks-actions
Approved by: michaelwoerister
Pushing d63a8d9 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Jan 10, 2022
@bors bors merged commit d63a8d9 into rust-lang:master Jan 10, 2022
@rustbot rustbot added this to the 1.60.0 milestone Jan 10, 2022
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (d63a8d9): comparison url.

Summary: This change led to moderate relevant regressions 😿 in compiler performance.

  • Moderate regression in instruction counts (up to 1.3% on incr-unchanged builds of clap-rs)

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

Next Steps: If you can justify the regressions found in this perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please open an issue or create a new PR that fixes the regressions, add a comment linking to the newly created issue or PR, and then add the perf-regression-triaged label to this PR.

@rustbot label: +perf-regression

@rylev
Copy link
Member

rylev commented Jan 12, 2022

This was already identified as being an acceptable performance hit since it is a correctness fix. I'll mark this as triaged for now.

@rustbot label: +perf-regression-triaged

@rustbot rustbot added the perf-regression-triaged The performance regression has been triaged. label Jan 12, 2022
This was referenced Feb 21, 2022
Closed
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bjorn3 bjorn3 bjorn3 left review comments

@michaelwoerister michaelwoerister michaelwoerister left review comments

Assignees

@michaelwoerister michaelwoerister

Labels
merged-by-bors This PR was explicitly merged by bors. perf-regression Performance regression. perf-regression-triaged The performance regression has been triaged. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
1.60.0
Development

Successfully merging this pull request may close these issues.

HashStable caching does not take into account StableHashingContext settings